fix: tighten recursive delete policy matching#3251
Conversation
Signed-off-by: arvidpeldan <peldans@gmail.com>
Signed-off-by: arvidpeldan <peldans@gmail.com>
Signed-off-by: arvidpeldan <peldans@gmail.com>
Signed-off-by: arvidpeldan <peldans@gmail.com>
Signed-off-by: arvidpeldan <peldans@gmail.com>
Signed-off-by: arvidpeldan <peldans@gmail.com>
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your contribution! 🎉 Your detailed explanation of the problem and solution is excellent, and the added test coverage is thorough. Before merging, please ensure:
For guidance, please refer to CONTRIBUTING.md. |
|
The branch is split by surface so each package change is easy to review on its own. The first commits apply the recursive-delete fix and matching tests to OpenCode, Claude Code, Copilot CLI, and Antigravity CLI separately. There’s also a small examples commit to keep the Copilot CLI example policies in sync with the package configs. The final commit is a review follow-up across the affected parsers. It tightens the flag handling for longer Unix clusters like |
|
@microsoft-github-policy-service agree company="Regent" |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Reviewed across all four bundles (claude-code, opencode, copilot-cli, antigravity) plus the examples/copilot-cli-agt config mirror. The design is sound and fail-safe: the regex is reduced to a broad delete-command trigger (^|[\s;&|()])(?:rm|remove-item|...)\b and the actual deny decision is delegated to the new hasRecursiveForceDelete() parser. shouldBypassBlockedCommandRule now denies only when the command is genuinely recursive and force AND is not a safe-cleanup target.
I worked through the notable edge cases in getRmCommandDetails:
- Split / reordered / clustered short options (
rm -r -f,rm -fr,rm -rfv,rm target -rf), long options (--recursive --forcein either order), PowerShell-Recurse/-Forceand their unambiguous abbreviations (-r,-fo), and Windowsrd /s /qall resolve to recursive+force → deny. -falone is correctly gated out as force (the PowerShell force check requires length >= 2), andnode_modules/build/dist,rm -f,rm --recursivealone,-Confirm/-Filtercorrectly fall through as not-denied.
Importantly this does not introduce a fail-open: a destructive form the parser fails to classify reverts to the normal shell-tool review path rather than being auto-allowed, and the leading (^|[\s;&|()]) anchor prevents substring false-matches (charm, warm). The PR also correctly identifies the pre-existing \b-rf\b word-boundary bug that meant the canonical rm -rf target never fired in several bundles — this genuinely fixes that gap.
Tests are comprehensive (positive deny + negative allow lists) and mirrored per package; CI green. Careful, well-scoped security hardening. LGTM.
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Regressions vs the previous denylist:
- Left-anchor:
(^|[\s;&|()])misses`rm -rf /`and{rm -rf /;}which the old\bcaught. Add`and{to the class in all 14 JSONs. - Recursive-without-force is now allowed on antigravity/copilot (a test asserts
rm --recursive important-datapasses). That is a loosening; restore the deny or own it explicitly in the title/body. isUnixRmShortOptionClusterfails open on unknown letters (rm -rfI fooslips through). Invert to single-dash-letters-only.- Restore the trailing newline in
agent-governance-opencode/lib/policy.mjs.
Follow-up to the recursive-delete hardening, addressing review feedback on PR microsoft#3251. Applied to all four bundles (claude-code, opencode, copilot-cli, antigravity) plus the examples config mirror. - Trigger anchor: add ` and { to the delete-command character class in all 14 policy JSONs so `rm -rf /` and {rm -rf /;} are matched, which the previous class dropped. Also add normalizeCommandNameToken() so the parser un-glues leading `, {, (, $ from the command token; without it a matched trigger still bypassed the deny (commandIndex === -1). - Recursive-without-force: restore the deny. hasRecursiveForceDelete -> hasRecursiveDelete now gates on recursive alone (force is still parsed but no longer required), so `rm --recursive`, `rm -r`, and `Remove-Item -Recurse` are denied again unless the target is a safe-cleanup dir. - Short-option cluster: invert isUnixRmShortOptionCluster from the allow-list /^-[dfiprvw]+$/ to letters-only /^-[a-z]+$/, so a cluster with an unanticipated letter (e.g. `rm -rfx foo`) no longer fails open and drops its r/f flags. - Restore the trailing newline in agent-governance-opencode/lib/policy.mjs. Tests updated per bundle: recursive-only and delimiter-wrapped forms moved to the deny lists, a genuine fail-open cluster (-rfx) added as a regression guard, and the artificial bash -Confirm negative replaced with `rm -i` (PowerShell -Confirm/-Filter negatives kept under their proper surface). All four suites pass (npm run check, exit 0). Signed-off-by: arvidpeldan <peldans@gmail.com>
🤖 AI Agent: test-generator — `policy.mjs`
|
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. The PR improves recursive delete detection but has a minor test coverage gap.
Action items: None. Warnings:
|
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
🤖 AI Agent: security-scanner — View details
No security issues found. |
Thanks! All four addressed in
|
Signed-off-by: arvidpeldan <peldans@gmail.com>
3 participants
Description
The recursive-delete deny rule depended too much on narrow command regexes such as
rm -rf. Before this change, equivalent recursive deletes likerm -fr important-data,rm -r -f important-data,rm --recursive important-data,Remove-Item -Recurse important-data, andrd /s /q important-datacould miss the deny rule and fall through to the normal shell-tool review path instead.One root cause was that several old rules matched text shape rather than command semantics. In the Claude Code, OpenCode, and Copilot CLI bundles the Bash rule used patterns like
\brm\b[\s\S]*\b-rf\b; in JavaScript regex semantics\b-rf\bdoes not matchrm -rf target(the space and-are both non-word characters), so the intended deny never fired for the canonical form. Antigravity's old pattern matched compactrm -rf/rm -frbut missed split/reordered forms (rm -r -f,rm --force --recursive,rm target -rf).This PR reduces the regex to a broad delete-command trigger and delegates the deny decision to a
hasRecursiveDelete()parser that inspects the actual flags, so equivalent destructive forms are denied consistently while safe cleanup targets are not.Deny semantics: a delete is denied when it is recursive, regardless of whether a force flag is present, unless every target is a known safe-cleanup directory (
node_modules,dist,build, …). Force is still parsed but is not required to deny. Non-recursive deletes (rm -f,rm -i,rm --force) are not hard-denied by this rule.Notable cases covered:
rm -rf,rm -fr,rm -rfv, reorderedrm important-data -rfrm -r -f,rm --recursive,rm --recursive --force(either order)rm -r,rm --recursive,Remove-Item -RecurseRemove-Item -Recurse -Force, abbreviationsRemove-Item -r -fo,ri -r -ford /s /q,del … /s /q`rm -rf /`,{rm -rf /;},$(rm -rf /)rm -rfx foorm -rf node_modules,Remove-Item -Recurse -Force build), and non-recursive deletes (rm -f,rm -i)Type of Change
Package(s) Affected
Checklist
Attribution & Prior Art
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
Codex helped add regression tests, find the same parser issue in sibling packages, and carry the reviewed fix across those packages. I reviewed the implementation and test coverage.
IP, Patents, and Licensing